-
Notifications
You must be signed in to change notification settings - Fork 143
Use more appropriate asserts #259
base: master
Are you sure you want to change the base?
Conversation
Generated by 🚫 danger |
Current coverage is 93.43% (diff: 100%)@@ master #259 diff @@
==========================================
Files 72 72
Lines 5015 5015
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 4682 4686 +4
+ Misses 333 329 -4
Partials 0 0
|
@spotify/objc-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time we actually want to make sure that the same instance of an object is used, not just that they're equal. So I'd update only the index path cases that were failing (and should use XCTAssertEqualObjects
).
@@ -58,7 +58,7 @@ - (void)testRegisteringFactoryAndCreatingAction | |||
HUBActionFactoryMock * const factory = [[HUBActionFactoryMock alloc] initWithActions:@{actionIdentifier.namePart: action}]; | |||
[self.actionRegistry registerActionFactory:factory forNamespace:actionIdentifier.namespacePart]; | |||
|
|||
XCTAssertEqual([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action); | |||
XCTAssertEqualObjects([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we actually want to make sure that the objects are the same instance so XCTAssertEqual
should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should use XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action)
for that then, just to be explicit about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hamcrest has a "same instance" matcher that is good for these scenarios. XCTest doesn't seem to though :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it more explicit to use XCTAssertEqual
that does exactly that? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, possibly. I have a problem with XCTAssertEqual
as it's often misused, and people often aren't asserting what they think. If you do want to test instance equality, then using XCTAssertEqual
just feels wrong as actually tests "more" than equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are testing instance equality XCTAssertTrue(a == b)
is far more explicit. XCTAssertEqual
is just too vague and has different behaviour depending on what you pass it, and that irks me :)
XCTAssertEqual([self.actionRegistry createCustomActionForIdentifier:actionIdentifier], action); | ||
|
||
// These should be the same instance. | ||
XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really fail to understand why this is better than XCTAssertEqual
which is meant for exactly these type of comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Ceri; XCTAssertEqual
is a smell for non-scalar types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's because in a test, if I look at a line that says XCTAssertEqual(objA, obj2)
, I immediately think "do we really want to check equality here, or is this a typo".
Admittedly XCTAssertTrue(obj1 == obj2)
doesn't do a great deal to fix that ambiguity, which is why I like Hamcrest's approach of providing a "same instance" matcher for objects so that it's obvious by looking at it whether you want to check for equality or instance equivalence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment is the most useful thing here. I'll revert the assert and keep the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a description to the assert? That is:
XCTAssertTrue([self.actionRegistry createCustomActionForIdentifier:actionIdentifier] == action,
@"The action should be the same instance as the factory mock vends");
That way the comment will be directly attached to the assert. Making sure it also shows up when the test fails.
@JohnSundell : I would assume that by default we'd want to be checking object equality in tests, unless we have a requirement that something returns the same instance under certain conditions. Only then would we want to test the actual instances. The Only fixing the cases with |
@cerihughes |
@@ -108,8 +108,8 @@ - (void)testTwoSubsequentRenders | |||
[self waitForExpectationsWithTimeout:2 handler:nil]; | |||
|
|||
XCTAssertEqual([self.collectionViewLayout numberOfInvocations], 2u); | |||
XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel); | |||
XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel); | |||
XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t we use XCTAssertTrue([self.collectionViewLayout capturedViewModelAtIndex:0] == firstViewModel)
here as well? As well as adding an assert description stating that we really do want to check for instance equality.
Just like at https://github.com/spotify/HubFramework/pull/259/files#diff-51a93e5f57aabdf96df5027e93c75d58R62
XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel); | ||
XCTAssertEqualObjects([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel); | ||
XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:0], firstViewModel); | ||
XCTAssertEqual([self.collectionViewLayout capturedViewModelAtIndex:1], secondViewModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cerihughes I’m in favor of the changes made in this PR. Unless we explicitly want to check that two objects are the same instance we should use Further I think we should, like you’re doing here, use |
Always use
XCTAssertEqualObjects
when comparing objects for equality.XCTAssertEqual
was causing some issues withNSIndexPath
equality on 32 bit architectures (possibly because 64-bit architectures pool instances that are equal?)